Skip to content

TRT-2796: Revert #8997 "OCPBUGS-98461: requeue CRR on transiently unavailable resources" - #9034

Closed
redhat-chai-bot wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:revert-pr-8997
Closed

TRT-2796: Revert #8997 "OCPBUGS-98461: requeue CRR on transiently unavailable resources"#9034
redhat-chai-bot wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:revert-pr-8997

Conversation

@redhat-chai-bot

@redhat-chai-bot redhat-chai-bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Why

Payload 5.0.0-0.ci-2026-07-16-215225 has the hypershift-e2e-aks blocking job failing consistently. This is the 3rd consecutive non-accepted 5.0 CI payload.

AI payload analysis identified PR #8997 at 100% confidence as the root cause.

Root Cause

PR #8997 changed requeueSoon=false to requeueSoon=true in 6 return paths in certificaterevocationcontroller.go. On HA clusters with multiple CRRs, this causes excessive requeue churn that prevents certificate revocation from completing within the 10-minute timeout. TestCreateClusterHABreakGlassCredentials fails consistently at the revocation step.

What This Reverts

This reverts merge commit 13f579d (PR #8997), restoring the original requeueSoon=false behavior in all 6 return paths and removing the associated tests.

References

cc @jparrill

/override ci/prow/e2e-aks
/override ci/prow/e2e-aks-ovn-conformance-serial
/label acknowledge-critical-fixes-only


@sosiouxme requested in Slack thread

Summary by CodeRabbit

  • Bug Fixes

    • Updated certificate revocation processing to avoid unnecessary reconciliation retries when required signer, secret, or trust-bundle data is unavailable.
    • Preserved existing validation and successful revocation behavior.
  • Tests

    • Removed outdated test cases covering forced retries for temporarily unavailable resources.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 17, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 17, 2026

Copy link
Copy Markdown

@redhat-chai-bot: This pull request references TRT-2796 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Why

Payload 5.0.0-0.ci-2026-07-16-215225 has the hypershift-e2e-aks blocking job failing consistently. This is the 3rd consecutive non-accepted 5.0 CI payload.

AI payload analysis identified PR #8997 at 100% confidence as the root cause.

Root Cause

PR #8997 changed requeueSoon=false to requeueSoon=true in 6 return paths in certificaterevocationcontroller.go. On HA clusters with multiple CRRs, this causes excessive requeue churn that prevents certificate revocation from completing within the 10-minute timeout. TestCreateClusterHABreakGlassCredentials fails consistently at the revocation step.

What This Reverts

This reverts merge commit 13f579d (PR #8997), restoring the original requeueSoon=false behavior in all 6 return paths and removing the associated tests.

References

cc @jparrill

/override ci/prow/e2e-aks
/override ci/prow/e2e-aks-ovn-conformance-serial
/label acknowledge-critical-fixes-only


@sosiouxme requested in Slack thread

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci Bot added acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. do-not-merge/needs-area labels Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 4c260d94-8014-4a97-9fb8-1bf1e5a7cfaa

📥 Commits

Reviewing files that changed from the base of the PR and between 3b49a4a and 36a3506.

📒 Files selected for processing (2)
  • control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go
  • control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
💤 Files with no reviewable changes (1)
  • control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go

📝 Walkthrough

Walkthrough

The certificate revocation controller now returns requeue=false when signer mappings, signer secrets, or the total client trust bundle are unavailable in both revocation steps. Existing success and validation logic remains unchanged. Direct tests covering requeue behavior for unavailable resources were removed from the controller test suite.

Possibly related PRs

Suggested reviewers: jparrill, sdminonne

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: reverting PR #8997 and restoring the prior CRR requeue behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The remaining test titles are static, descriptive strings; the reverted subtests remove the only potentially risky names.
Test Structure And Quality ✅ Passed No new Ginkgo tests were added; the PR only deletes unit subtests and the remaining tests use fake clients, no waits, and no cluster resources.
Topology-Aware Scheduling Compatibility ✅ Passed Only requeue-flag reversions and test deletions; no pod specs, nodeSelectors, affinity, spread constraints, or ControlPlaneTopology logic were added.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the PR only removes unit-test subcases and changes controller requeue flags, with no IPv4 or external-connectivity assumptions introduced.
No-Weak-Crypto ✅ Passed Touched files only change requeue logic/tests; scans found SHA-256 only, with no MD5/SHA1/DES/RC4/3DES/ECB, custom crypto, or secret-comparison code.
Container-Privileges ✅ Passed PR only changes requeue booleans and removes tests; no container/K8s manifests or privileged/securityContext/host* fields are introduced.
No-Sensitive-Data-In-Logs ✅ Passed The PR only changes requeue return values and removes tests; no new log statements or sensitive data exposures were introduced.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the area/control-plane-pki-operator Indicates the PR includes changes for the control plane PKI operator - in an OCP release label Jul 17, 2026
@openshift-ci
openshift-ci Bot requested review from devguyio and enxebre July 17, 2026 10:43
@openshift-ci

openshift-ci Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@redhat-chai-bot: redhat-chai-bot unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers.

Details

In response to this:

Why

Payload 5.0.0-0.ci-2026-07-16-215225 has the hypershift-e2e-aks blocking job failing consistently. This is the 3rd consecutive non-accepted 5.0 CI payload.

AI payload analysis identified PR #8997 at 100% confidence as the root cause.

Root Cause

PR #8997 changed requeueSoon=false to requeueSoon=true in 6 return paths in certificaterevocationcontroller.go. On HA clusters with multiple CRRs, this causes excessive requeue churn that prevents certificate revocation from completing within the 10-minute timeout. TestCreateClusterHABreakGlassCredentials fails consistently at the revocation step.

What This Reverts

This reverts merge commit 13f579d (PR #8997), restoring the original requeueSoon=false behavior in all 6 return paths and removing the associated tests.

References

cc @jparrill

/override ci/prow/e2e-aks
/override ci/prow/e2e-aks-ovn-conformance-serial
/label acknowledge-critical-fixes-only


@sosiouxme requested in Slack thread

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.16%. Comparing base (3b49a4a) to head (36a3506).

Files with missing lines Patch % Lines
...ationcontroller/certificaterevocationcontroller.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9034      +/-   ##
==========================================
- Coverage   44.18%   44.16%   -0.02%     
==========================================
  Files         772      772              
  Lines       96340    96340              
==========================================
- Hits        42565    42550      -15     
- Misses      50831    50841      +10     
- Partials     2944     2949       +5     
Files with missing lines Coverage Δ
...ationcontroller/certificaterevocationcontroller.go 56.00% <0.00%> (-1.69%) ⬇️
Flag Coverage Δ
cmd-support 38.24% <ø> (ø)
cpo-hostedcontrolplane 46.27% <ø> (ø)
cpo-other 45.22% <ø> (ø)
hypershift-operator 54.14% <ø> (ø)
other 32.19% <0.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jparrill

Copy link
Copy Markdown
Contributor

/approve

@jparrill

Copy link
Copy Markdown
Contributor

/lgtm

@jparrill

Copy link
Copy Markdown
Contributor

/verified by e2e

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Jul 17, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@jparrill: This PR has been marked as verified by e2e.

Details

In response to this:

/verified by e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aks
/test e2e-aws
/test e2e-aws-upgrade-hypershift-operator
/test e2e-azure-v2-self-managed
/test e2e-kubevirt-aws-ovn-reduced
/test e2e-v2-aws
/test e2e-v2-gke

@openshift-ci

openshift-ci Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jparrill, redhat-chai-bot

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2026
@neisw

neisw commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

/test e2e-aws
/test e2e-aws-upgrade-hypershift-operator

@neisw

neisw commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

/hold
for more payload evaluation

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2026
@cwbotbot

cwbotbot commented Jul 17, 2026

Copy link
Copy Markdown

Test Results

e2e-aws

e2e-aks

@neisw

neisw commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

/retest-required

1 similar comment
@neisw

neisw commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

/retest-required

@openshift-ci

openshift-ci Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

@redhat-chai-bot: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-v2-gke 36a3506 link true /test e2e-v2-gke

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@jparrill

Copy link
Copy Markdown
Contributor

Analysis of the two cited periodic runs

Run 1 (Jul 19 18:46 UTC): TestCreateClusterHABreakGlassCredentials fails. The CRR for sre-break-glass is stuck at:

  • PreviousCertificatesRevoked=False: WaitingForAvailable(Previous signer certificate not yet revoked.)
  • NewCertificatesTrusted=True: AsExpected(New signer certificate trusted.)

This means the CRR passed the propagation phase (ensureNewSignerCertificatePropagated) successfully and is stuck in the revocation phase (ensureOldSignerCertificateRevoked), specifically at the KAS trust bundle verification — the normal revocation path, not a nil-resource path.

Run 2 (Jul 19 20:28 UTC): TestCreateClusterHABreakGlassCredentials passes. The failures in this run are unrelated: Karpenter Kubelet propagation (TLS error) and NodePool Teardown timeout.

Why #8997 is not the cause

PR #8997 changed requeueSoon from false to true in 6 return paths. All 6 paths share the same characteristic: they execute only when a resource is transiently nil — the signer secret doesn't exist yet, or the trust bundle ConfigMap hasn't been created. These are guard clauses that return early before any revocation logic runs:

// ensureNewSignerCertificatePropagated
if signers == nil { return true, nil, true, nil }           // signer secret nil
if totalClientTrustBundle == nil { return true, nil, true, nil }  // trust bundle nil

// ensureOldSignerCertificateRevoked
if signerSecret == nil { return true, nil, true, nil }      // signer secret nil
if totalClientTrustBundle == nil { return true, nil, true, nil }  // trust bundle nil

The CRR in Run 1 has NewCertificatesTrusted=True, which means ensureNewSignerCertificatePropagated completed successfully — the signer secret and trust bundle both existed and were processed. The failure is in ensureOldSignerCertificateRevoked at the KAS verification step (checking that KAS has loaded the updated trust bundle and rejects the old certificate). This is the normal revocation path that was not changed by #8997.

If our nil-resource requeue was causing "excessive churn", it would only churn when the resources don't exist — which is not the case here (they exist, the propagation phase proved it). And it would fail consistently — Run 2 passing disproves systematic churn.

What the failure actually is

This is the same intermittent CRR timeout that existed before #8997 — the exact bug we were trying to fix. The CRR condition WaitingForAvailable(Previous signer certificate not yet revoked.) means the controller is polling KAS pods to verify they've loaded the new trust bundle, and this verification didn't complete within 10 minutes. This happened in 3/8 runs before our PR was merged (documented in the OCPBUGS-98461 bug report).

The correlation is temporal (our PR merged Jul 16, failure on Jul 19) but not causal — the failure path is in code we did not modify.


I'd ask to keep the hold on this revert for now while we gather more data. If more runs confirm the pattern is unchanged (intermittent, not consistent), this revert would reintroduce the original stalling bug without fixing the observed failure.

@neisw

neisw commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

/close

Due to the cited analysis and improved job performance noted in https://redhat.atlassian.net/browse/TRT-2796?focusedCommentId=17657265

@openshift-ci openshift-ci Bot closed this Jul 20, 2026
@openshift-ci

openshift-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@neisw: Closed this PR.

Details

In response to this:

/close

Due to the cited analysis and improved job performance noted in https://redhat.atlassian.net/browse/TRT-2796?focusedCommentId=17657265

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane-pki-operator Indicates the PR includes changes for the control plane PKI operator - in an OCP release do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants